Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/reports #22

Merged
merged 10 commits into from
Mar 24, 2024
Merged

Feature/reports #22

merged 10 commits into from
Mar 24, 2024

Conversation

JonathanDuvalV
Copy link
Collaborator

Added two new routes, one public route for reporting an event
image

and a protected route for moderators to fetch the reports
image

Copy link
Contributor

@MysticFragilist MysticFragilist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aussi, j'ai deux comments sur les reports:

1- Il faudrait que le endpoint pour créer un Report soit RateLimited sur plusieurs valeurs:
- Un temps de Window configurable (.env): https://www.c-sharpcorner.com/blogs/rate-limiting-in-net-70
- S'assurer d'essayer du mieux possible de réduire les requêtes en double, (par exemple faire un check sur l'adresse IP pourrait être possible mais pt pas pratique si 70% des gens report depuis le réseau de l'école). On pourra en reparler je sais pas si c'est faisable. Exemple: https://stackoverflow.com/questions/76309904/net-7-rate-limiting-rate-limit-by-ip
- Peut-être avoir 10 secondes par défaut et un max PermitLimit de 4 mettons? donc 1 report au 2.5s environ. Si c'est configurable après ça peut être worth je pense.

2- Je me demandais si on voulait pas avoir un genre de check s'il y a plus de X reports (par exemple 10) sur la même annonce on envoit un courriel au modérateur pour aller review l'annonce pour être sûr de sa validité. Il faudrait qu'une fois qu'un courriel se fait envoyer au modérateur, il ne se renvoit plus jamais.

Si jamais le point 2 est trop chiant, tu peux créer une issue pour le faire mais je pense le point 1 faudrait le faire live si on veut avoir le endpoint online.

[HttpGet("reports")]
public ActionResult<IEnumerable<ReportResponseDTO>> GetEventsReports()
{
EnsureIsModerator();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quand la PR avec les policy va etre merger, je vais retirer ca!

@JonathanDuvalV
Copy link
Collaborator Author

1- Il faudrait que le endpoint pour créer un Report soit RateLimited sur plusieurs valeurs: - Un temps de Window configurable (.env): https://www.c-sharpcorner.com/blogs/rate-limiting-in-net-70 - S'assurer d'essayer du mieux possible de réduire les requêtes en double, (par exemple faire un check sur l'adresse IP pourrait être possible mais pt pas pratique si 70% des gens report depuis le réseau de l'école). On pourra en reparler je sais pas si c'est faisable. Exemple: https://stackoverflow.com/questions/76309904/net-7-rate-limiting-rate-limit-by-ip - Peut-être avoir 10 secondes par défaut et un max PermitLimit de 4 mettons? donc 1 report au 2.5s environ. Si c'est configurable après ça peut être worth je pense.

Pour le duplicate, je crois pas que c'est possible avec le rate limiter de .NET. J'ai ajouté un AvoidDuplicate dans le ReportService qui vérifie si un report avec une raison identique sur la même publication a été envoyé dans le même time window que le rate limiter.

Copy link
Contributor

@MysticFragilist MysticFragilist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mini comment sinon all good! LGTM quand t'auras fix!!

@JonathanDuvalV JonathanDuvalV merged commit d525d11 into main Mar 24, 2024
2 checks passed
@JonathanDuvalV JonathanDuvalV deleted the feature/reports branch March 24, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants